Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MCO-1521: Promote PinnedImageSet to GA #2198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

MCO-1521: Promote PinnedImageSet to GA #2198

wants to merge 1 commit into from

Conversation

RishabhSaini
Copy link

Opened for testing

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 11, 2025

Hello @RishabhSaini! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 11, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 11, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RishabhSaini
Once this PR has been reviewed and has the lgtm label, please assign sjenning for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@RishabhSaini RishabhSaini changed the title Promote PinnedImageSet to GA MCO-1521: Promote PinnedImageSet to GA Feb 11, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 11, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 11, 2025 •
edited by openshift-ci bot
Loading

@RishabhSaini: This pull request references MCO-1521 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Opened for testing

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@RishabhSaini RishabhSaini force-pushed the pis branch 3 times, most recently from 4382c84 to c6dc790 Compare February 11, 2025 23:19
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 20, 2025

@RishabhSaini: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/verify cc6099e link true /test verify

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

yuqi-zhang
yuqi-zhang reviewed Feb 24, 2025
View reviewed changes
Copy link
Contributor

@yuqi-zhang yuqi-zhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is ready, some questions/suggestions inline

machineconfiguration/v1/types_pinnedimageset.go
// +kubebuilder:validation:MaxLength=447
// +kubebuilder:validation:XValidation:rule=`self.split('@').size() == 2 && self.split('@')[1].matches('^sha256:[a-f0-9]{64}$')`,message="the OCI Image reference must end with a valid '@sha256:<digest>' suffix, where '<digest>' is 64 characters long"
// +kubebuilder:validation:XValidation:rule=`self.split('@')[0].matches('^([a-zA-Z0-9-]+\\.)+[a-zA-Z0-9-]+(:[0-9]{2,5})?/([a-zA-Z0-9-_]{0,61}/)?[a-zA-Z0-9-_.]*?$')`,message="the OCI Image name should follow the host[:port][/namespace]/name format, resembling a valid URL without the scheme"
Name string `json:"name"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: within our MachineOSConfig object, we have a type (with validation) called ImageDigestFormat now. I wonder if it's worth it to consolidate them and have type validation the same across all MCO references for digest based images

hack/update-payload-crds.sh
@@ -24,7 +24,6 @@ crd_globs="\
operator/v1/zz_generated.crd-manifests/0000_50_openshift-controller-manager_02_openshiftcontrollermanagers*.crd.yaml
machineconfiguration/v1/zz_generated.crd-manifests/*.crd.yaml
machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes*.crd.yaml
machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_pinnedimagesets*.crd.yaml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is telling the payload to remove the v1alpha1 CRDs and install the v1 CRDs, which I think will break techpreview since the MCO wouldn't know how to process these in the meantime?

i.e. I think we would need to create the v1 API through this PR, and then still install alpha CRDs for now, then move the MCO to process v1 API, and simultaneously have the CRDs switch to v1 here as a second step.

machineconfiguration/v1/types_pinnedimageset.go
// conditions represent the observations of a pinned image set's current state.
// +patchMergeKey=type
// +patchStrategy=merge
// +kubebuilder:validation:MaxItems=10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing a make update locally causes this to regen, so I think this might be missing from the generated CRs

@RishabhSaini
Copy link
Author

Here are some outstanding question after our discussion with Sam on this:

  1. What are some PIS Status.Conditions needed?
  2. Should we have PIS Status Conditions defined explicitly as constants in this API or should we have them as Go objects instead and populate them somewhere in the MCO
  3. How would we differentiate the Status reporting between PIS and MCN?
  4. For the MCN tracking the PIS in MachineConfigNodeStatusPinnedImageSet, we might not be using LastFailedGeneration and LastFailedGenerationErrors as intended, and so what should they be changed to?

@cgwalters
Copy link
Member

Note there's some overlap between PIS and bootc's logically bound images especially as it relates to OCL (lots of TLAs here!).

A neat advantage of LBIs is that they live underneath a bootc-owned readonly bind mount, so even manually running podman image rm won't work, whereas AFAIK today podman is totally unaware of crio's attempt to pin and can still remove (although I could be wrong, maybe crio makes a running stub container at least to block that?).

@RishabhSaini
Copy link
Author

RishabhSaini commented Feb 24, 2025 •
edited
Loading

Ok just verified, Pinned Images defined by CRIO can be deleted by Podman. Despite both using the storage as containers-storage, they don't seem to be aware of the pinned Images. Either a lower level mechanism in container/storage is needed or we could explore LBI's

[root@crc ~]# cat /etc/crio/crio.conf.d/50-pinned-images && crictl images --digests --pinned | grep true
[crio]
  [crio.image]
    pinned_images = ["quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:479f8a99cbe432551448776965aac1f44501c08aa01539d77ab5976fdbbe1c83", "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:86d26e7ebcccd6f07a75db5b1e56283b25c2ee1c6a755d6ffc5a4d59beb9c504", "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:e43b2ef4fbc42dbcbea5d67f57f3feed38f6b45fb712c99acb06490103e277a9"]
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              3782924b74905       a3a9b9964565f       378MB               true
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              479f8a99cbe43       5f2e4eb4b7324       519MB               true
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              86d26e7ebcccd       7948809a7f475       672MB               true
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              e43b2ef4fbc42       6d27b7ed537e2       826MB               true

[root@crc ~]# podman rmi quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:479f8a99cbe432551448776965aac1f44501c08aa01539d77ab5976fdbbe1c83
Untagged: quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:479f8a99cbe432551448776965aac1f44501c08aa01539d77ab5976fdbbe1c83
Deleted: 5f2e4eb4b7324f29726100dda917d601d6ac822699301c22a63cf326c73330cf

@RishabhSaini
Copy link
Author

created a bug here: https://issues.redhat.com/browse/OCPBUGS-51284

yuqi-zhang
yuqi-zhang reviewed Feb 26, 2025
View reviewed changes
machineconfiguration/v1/types_pinnedimageset.go
// +listType=map
// +listMapKey=type
// +optional
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking through this a bit more, since we can have (500?) pinned images, should we have a status corresponding to each pinned image instead of one general conditions? So multiple failures can be reflected properly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, maybe not, since the PinnedImageSet would then have the reflect error for every node trying to update to it, will think through this a bit more via the MCN PR

@hexfusion
Copy link
Contributor

Ok just verified, Pinned Images defined by CRIO can be deleted by Podman. Despite both using the storage as containers-storage, they don't seem to be aware of the pinned Images. Either a lower level mechanism in container/storage is needed or we could explore LBI's

[root@crc ~]# cat /etc/crio/crio.conf.d/50-pinned-images && crictl images --digests --pinned | grep true
[crio]
  [crio.image]
    pinned_images = ["quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:479f8a99cbe432551448776965aac1f44501c08aa01539d77ab5976fdbbe1c83", "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:86d26e7ebcccd6f07a75db5b1e56283b25c2ee1c6a755d6ffc5a4d59beb9c504", "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:e43b2ef4fbc42dbcbea5d67f57f3feed38f6b45fb712c99acb06490103e277a9"]
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              3782924b74905       a3a9b9964565f       378MB               true
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              479f8a99cbe43       5f2e4eb4b7324       519MB               true
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              86d26e7ebcccd       7948809a7f475       672MB               true
quay.io/openshift-release-dev/ocp-v4.0-art-dev                                 <none>              e43b2ef4fbc42       6d27b7ed537e2       826MB               true

[root@crc ~]# podman rmi quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:479f8a99cbe432551448776965aac1f44501c08aa01539d77ab5976fdbbe1c83
Untagged: quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:479f8a99cbe432551448776965aac1f44501c08aa01539d77ab5976fdbbe1c83
Deleted: 5f2e4eb4b7324f29726100dda917d601d6ac822699301c22a63cf326c73330cf

LBI isn't released yet and would be implementation details. I don't see that blocking the API promotion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@yuqi-zhang yuqi-zhang yuqi-zhang left review comments

@isabella-janssen isabella-janssen isabella-janssen left review comments

@cgwalters cgwalters Awaiting requested review from cgwalters

@sinnykumari sinnykumari Awaiting requested review from sinnykumari

Assignees
No one assigned
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RishabhSaini @openshift-ci-robot @cgwalters @hexfusion @JoelSpeed @yuqi-zhang @isabella-janssen